Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[composable-controller] Narrow class and messenger types by parameterizing over child controllers list #3952

Merged
merged 26 commits into from
May 30, 2024
Merged

[composable-controller] Narrow class and messenger types by parameterizing over child controllers list #3952

merged 26 commits into from
May 30, 2024

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Feb 22, 2024 •
edited
Loading

Explanation

Currently, the allow lists of ComposableControllerMessenger are set to string, which is too permissive. Each ComposableController instance needs typing and allow lists that are specific to its set of input child controllers.

To achieve this, the ComposableController class and its messenger are made polymorphic upon the ControllerState type, which defines the shape of the composed state.

References

Changelog

@metamask/composable-controller

Added

  • Adds and exports new types: (#3952)
    • RestrictedControllerMessengerConstraint, which is the narrowest supertype of all controller-messenger instances.
    • LegacyControllerStateConstraint, a universal supertype for the controller state object, encompassing both BaseControllerV1 and BaseControllerV2 state.
    • ComposableControllerStateConstraint, the narrowest supertype for the composable controller state object.

Changed

  • BREAKING: The ComposableController class is now a generic class that expects one generic argument ComposableControllerState (#3952).
    • BREAKING: For the ComposableController class to be typed correctly, any of its child controllers that extend BaseControllerV1 must have an overridden name property that is defined using the as const assertion.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@MajorLift MajorLift changed the base branch from main to 240222-composable-controller-type-fixes February 23, 2024 04:37
@MajorLift MajorLift force-pushed the 240220-composable-controller-narrow-typing branch from 8fb745a to feb6c38 Compare February 23, 2024 04:37
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch 2 times, most recently from 0c830ba to 599fd44 Compare February 23, 2024 04:48
@MajorLift MajorLift force-pushed the 240220-composable-controller-narrow-typing branch 5 times, most recently from ba71fbf to 923d77d Compare February 23, 2024 05:30
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from bdc167c to 0882483 Compare February 23, 2024 19:30
@MajorLift MajorLift force-pushed the 240220-composable-controller-narrow-typing branch 5 times, most recently from 716af2a to df1cdc8 Compare February 27, 2024 19:04
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from 6ef730e to 0fa97c3 Compare February 27, 2024 23:33
@MajorLift MajorLift force-pushed the 240220-composable-controller-narrow-typing branch from df1cdc8 to b2b9059 Compare February 27, 2024 23:35
@MajorLift MajorLift changed the title WIP [composable-controller] Narrow class and messenger types by parameterizing over child controllers list Feb 28, 2024
@MajorLift MajorLift force-pushed the 240220-composable-controller-narrow-typing branch 2 times, most recently from 717c66f to 94019d1 Compare March 3, 2024 18:02
@MajorLift MajorLift self-assigned this Mar 3, 2024
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from 0ab242a to 0fa97c3 Compare March 6, 2024 16:34
@MajorLift MajorLift force-pushed the 240220-composable-controller-narrow-typing branch from 94019d1 to 3e9a5d2 Compare March 6, 2024 16:36
@MajorLift MajorLift force-pushed the 240222-composable-controller-type-fixes branch from 0fa97c3 to 7d46b06 Compare March 7, 2024 18:26
Base automatically changed from 240222-composable-controller-type-fixes to main March 7, 2024 21:24
MajorLift added 13 commits April 25, 2024 11:46
@MajorLift MajorLift force-pushed the 240220-composable-controller-narrow-typing branch from 5f59bf9 to 58ba8bc Compare April 25, 2024 16:02
@MajorLift MajorLift marked this pull request as ready for review April 25, 2024 16:03
@MajorLift MajorLift requested a review from a team as a code owner April 25, 2024 16:03
@desi desi requested a review from a team May 23, 2024 19:19
@mcmire mcmire self-requested a review May 28, 2024 16:06
mcmire
mcmire reviewed May 28, 2024
View reviewed changes
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had some questions, but overall makes sense.

packages/composable-controller/CHANGELOG.md Outdated
@@ -25,10 +25,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Changed

- **BREAKING:** Passing a non-controller into `controllers` constructor option now throws an error ([#3904](https://github.com/MetaMask/core/pull/3904))
- **BREAKING:** The `AllowedAction` parameter of the `ComposableControllerMessenger` type is narrowed from `string` to `never`, as `ComposableController` does not use any external controller actions ([#3904](https://github.com/MetaMask/core/pull/3904))
- **BREAKING:** The `AllowedActions` parameter of the `ComposableControllerMessenger` type is narrowed from `string` to `never`, as `ComposableController` does not use any external controller actions. ([#3904](https://github.com/MetaMask/core/pull/3904))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we are updating the changelog for an older version, is this intentional? Are there changes we forgot to include for this version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed to align with final changelog in PR description: 365aa02

packages/composable-controller/src/ComposableController.ts Outdated
Comment on lines 191 to 197
P extends keyof ComposableControllerState = keyof ComposableControllerState,
> = P extends string
? ComposableControllerState[P] extends StateConstraint
? { name: P; state: ComposableControllerState[P] }
: BaseControllerV1<
BaseConfig & Record<string, unknown>,
BaseState & ComposableControllerState[P]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Can we use a non-one-letter name here for clarify? Perhaps ControllerName?

Suggested change
P extends keyof ComposableControllerState = keyof ComposableControllerState,
> = P extends string
? ComposableControllerState[P] extends StateConstraint
? { name: P; state: ComposableControllerState[P] }
: BaseControllerV1<
BaseConfig & Record<string, unknown>,
BaseState & ComposableControllerState[P]
ControllerName extends keyof ComposableControllerState = keyof ComposableControllerState,
> = Key extends string
? ComposableControllerState[ControllerName] extends StateConstraint
? { name: ControllerName; state: ComposableControllerState[ControllerName] }
: BaseControllerV1<
BaseConfig & Record<string, unknown>,
BaseState & ComposableControllerState[ControllerName]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed here: 25f2f0b

mcmire
mcmire approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@MajorLift MajorLift merged commit f1ca83d into main May 30, 2024
147 checks passed
@MajorLift MajorLift deleted the 240220-composable-controller-narrow-typing branch May 30, 2024 15:24
@MajorLift MajorLift mentioned this pull request Jun 15, 2024
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcmire mcmire mcmire approved these changes

Assignees

@MajorLift MajorLift

Labels
team-wallet-framework
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[composable-controller] Make class and messenger generic upon child controllers
3 participants
@MajorLift @Gudahtt @mcmire